fix(scheduler): don't leak pipeline-only fit errors onto live jobs - #1950
Closed
Thezone-1 wants to merge 2 commits into
Closed
fix(scheduler): don't leak pipeline-only fit errors onto live jobs#1950Thezone-1 wants to merge 2 commits into
Thezone-1 wants to merge 2 commits into
Conversation
Pipeline-only allocation runs inside solver simulations (e.g. Reclaim) that are rolled back. allocateTask recorded pre-predicate fit errors, and allocateTasksOnNodeSet recorded job fit errors, directly on the live session job. Statement.Rollback does not restore fit errors, so simulation-only diagnostics leaked onto pending jobs and masked the authoritative Allocate reason. Skip task and job fit-error writes when isPipelineOnly, mirroring the existing FittingNode writeFittingDelta=!isPipelineOnly behavior. Closes kai-scheduler#1948 Signed-off-by: Thezone-1 <somoprovobhattacharjee@gmail.com>
Contributor
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Total coverage: 54.2% -> 53.4% (delta -0.80%) Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
📊 Performance Benchmark ResultsComparing PR (
|
Contributor
Author
|
@ Since you filed #1948, I wanted to flag the overlap with your own #1921 ("scope fit errors to authoritative allocation attempts") — that looks like it's tackling the same leak from the allocation-attempt side, whereas this PR guards the fit-error writes in Happy to go whichever way makes sense: land this as the targeted fix, rebase it on top of #1921, or close it out if #1921 already covers the case. Just let me know what you'd prefer. |
Collaborator
|
@Thezone-1 I haven't had time to look into this enough to form a solid opinion because this is a relatively minor issue. I hope to look into this next week. |
enoodle
reviewed
Aug 7, 2026
enoodle
left a comment
enoodle
left a comment
Collaborator
There was a problem hiding this comment.
I think that the topology plugin would still add JobFitErrors to the job and other plugins can do that too. (see pkg/scheduler/plugins/topology/job_filtering.go:40 for example)
Contributor
Author
|
yeah you're right, and it's not only topology. job_filtering.go writes in 5 places and RecordVictimInvariantPrePredicateFailure in action_eligibility.go does too. FittingNode is fine since it already checks writeFittingDelta, but nothing stops the next plugin from adding one more. so guarding call sites is a bit pointless, it only covers whatever I found today. the general version would be muting the writes on the PodGroupInfo for the duration of the pipeline-only attempt so it doesn't matter who writes them. I have that working locally but honestly I don't love it, it's a mutable flag on a shared object restored by defer, so an early return that skips the restore silently drops real fit errors, which is this bug backwards. you said this one is fairly minor anyway, and your #1921 gets at the same leak from the allocation-attempt side without needing any of that. so I'm fine just closing this if #1921 covers it. let me know which you'd rather, happy either way. |
Contributor
Author
|
closing this one out. you were right that guarding the call sites in allocate.go only covers the writers I happened to find, and the general version (muting fit-error writes on the PodGroupInfo for the duration of the pipeline-only attempt) trades a narrow gap for a mutable flag on a shared object, which I don't think is a good trade for an issue you called minor. #1921 gets at the same leak from the allocation-attempt side and doesn't need any of that, so it's the better home for this. the reproducer test in here (TestSchedulingCyclePreservesAllocateFitErrors in pkg/scheduler/actions/integration_tests/reclaim/reclaim_fit_errors_test.go) is yours from #1948 and still passes on main. happy to send it as a standalone test-only PR if you want the coverage while #1921 lands. thanks for the review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
AllocateJob(..., isPipelineOnly=true)runs inside solver simulations (Reclaim, preempt, consolidation) that are rolled back withStatement.Rollback. Two paths recorded fit errors directly on the live session job during those simulations:allocateTaskwrote a task fit error whenPrePredicateFnfailed.allocateTasksOnNodeSet→handleFailedTaskAllocationwrote job fit errors on any task failure.Statement.Rollbackrestores allocation state but not fit errors, so these simulation-only diagnostics leaked onto the pending job and could obscure the authoritative reason produced by the realAllocatepass. Someone inspecting a pending job's fit errors could then see a reason from a failed Reclaim simulation rather than why the scheduler actually couldn't place the job.This change skips task and job fit-error writes when
isPipelineOnly, mirroring the existingFittingNode(..., writeFittingDelta=!isPipelineOnly)behavior that already suppresses per-node fit deltas during simulation. The authoritativeAllocatepass (isPipelineOnly=false) records fit errors exactly as before.Related Issues
Fixes #1948
Checklist
Breaking Changes
None.
Additional Notes
Includes the reproducer from the issue (
TestSchedulingCyclePreservesAllocateFitErrors, authored by @enoodle): it runs a full action loop where Reclaim's pipeline-only simulation hits an injected pre-predicate failure, and asserts the job's task/job fit errors after all actions equal those produced by the initialAllocate.